Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Fix broken specs #328

Merged
merged 4 commits into from
Nov 18, 2022
Merged

Conversation

RyanofWoods
Copy link
Contributor

@RyanofWoods RyanofWoods commented Oct 28, 2022

Having a broken suite is blocking merging PRs as well as giving the
repository a bad reputation, even though the repositories' logic is
not broken.


I would be desirable to have different PRs for these commits, however,
unfortunately, they cannot be separated, as each commit fixes a spec,
and a green suite is needed to merge.

@RyanofWoods
Copy link
Contributor Author

For f68d314 I took inspiration for what you did in the PCP extension @elia. Let me know what you think
solidusio/solidus_paypal_commerce_platform@fc16fe7

Copy link
Member

@elia elia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@elia elia requested a review from kennyadsl October 28, 2022 14:47
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Left a question about Ruby bump, but overall it's great! ❤️

solidus_paypal_braintree.gemspec Outdated Show resolved Hide resolved
@RyanofWoods RyanofWoods force-pushed the ryanofwoods/fix-broken-specs branch 2 times, most recently from 39b49e0 to 7b59e87 Compare November 7, 2022 14:35
@RyanofWoods
Copy link
Contributor Author

Hey @elia 👋 Got any ideas on why the lint-code job is now failing?

The suite passed originally, but then after I removed the TargetRubyVersion from .rubocop.yml, it fails.

I don't understand how it can handle a version of nil for Rails on some jobs, but not on the lint-code one when generating the Gemfile lock 🤔
https://app.circleci.com/pipelines/github/solidusio/solidus_paypal_braintree/422/workflows/56d0ecc3-1b70-417e-9e89-ee3ce5dcbc30

@kennyadsl
Copy link
Member

@RyanofWoods please, rebase now 🙂

As linting warnings is included in the test suite, if introducing new
rules is not controlled, the suite will keep breaking upon new rules.
We don't usually use TargetRubyVersion in other extensions:
- https://github.com/solidusio/solidus_auth_devise/blob/4a4f9f213d45b77cead29f45bf5bfd0d2021ab46/.rubocop.yml
- https://github.com/solidusio/solidus_stripe/blob/993a43db00ae15984e505a6a4641db955c0eaf1b/.rubocop.yml

Also, this was causing a lint warning as the version did not match the
minimum Ruby version listed in the gemspec. TargetRubyVersion can't be
2.5 as it's not supported by Rubocop, and thus Rubocop shouldn't ditact
the gem's required version.
Due to the following PR, the Country factory no longer always returns
true for `states_required`. It can be true for the appropriate countries
that require a state in shipping addresses. Because of this, addresses
using a country where `states_required` is true, such as the US, need
a state in the Spree::Address record to be valid.
solidusio/solidus#4272
Cardinal, that handles "3D Secure" for Braintree made some changes to
how the legacy 3Ds test numbers are handled. Because of this, the
numbers were changed to trigger the correct scenarios needed. The
DOM of the Cardinal iFrame also changed and needed tweaking. Cardinal
also likes the expiration date to be January and 3 years ahead of
today's year.

References:
- https://cardinaldocs.atlassian.net/wiki/spaces/CCen/pages/400654355/3DS+1.0+Test+Cases
- https://cardinaldocs.atlassian.net/wiki/spaces/CCen/pages/903577725/EMV+3DS+Test+Cases#Test-Case-12%3A-Error-on-Authentication
@RyanofWoods RyanofWoods force-pushed the ryanofwoods/fix-broken-specs branch from 7b59e87 to e5a1a57 Compare November 17, 2022 12:59
@kennyadsl
Copy link
Member

@RyanofWoods

Hey @elia 👋 Got any ideas on why the lint-code job is now failing?

I see it's passing now!

@elia
Copy link
Member

elia commented Nov 18, 2022

Awesome 🙌

@elia elia merged commit c90efd8 into solidusio:master Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants